Interpreter block-count PGO for WebAssembly CoreCLR - #132721
pavelsavara wants to merge 32 commits into
Conversation
|
Azure Pipelines: Successfully started running 4 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
b304cb6 to
cbcdca6
Compare
|
Blazor WASM PGO profile/trace https://gist.github.com/pavelsavara/70de5d2c5a7575f35eba0a72fc9e0abb |
|
Azure Pipelines: Successfully started running 4 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate findings affect counter correctness, session-specific flushing, trace collection, and end-to-end validation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds CoreCLR WebAssembly interpreter block-count PGO instrumentation and browser-side EventPipe trace collection for dotnet-pgo/R2R workflows.
Changes:
- Adds WASM interpreter probes, shared PGO allocation, and configuration.
- Adds EventPipe flushing and
collectPgoTrace(). - Adds documentation, build integration, and end-to-end validation.
File summaries
| File | Reviewed change / final review note |
|---|---|
src/native/libs/System.Native.Browser/diagnostics/types.ts |
Adds the PGO EventPipe keyword. |
src/native/libs/System.Native.Browser/diagnostics/index.ts |
Exposes the PGO collector. |
src/native/libs/System.Native.Browser/diagnostics/dotnet-pgo-trace.ts |
Implements timed trace collection. moderate (1 vote): stale timers can stop a later session; associate the timer with its original session. |
src/native/libs/System.Native.Browser/diagnostics/diagnostic-server-js.ts |
Supports startup js://pgo tracing. moderate (1 vote): add coverage for startup registration and downloaded traces. |
src/native/libs/System.Native.Browser/diagnostics/client-commands.ts |
Defines the PGO EventPipe command. |
src/native/libs/Common/JavaScript/types/public-api.ts |
Declares the diagnostics API. |
src/native/libs/Common/JavaScript/loader/dotnet.d.ts |
Updates loader typings. |
src/native/eventpipe/ep.c |
Invokes the session-stopping hook. moderate (1 vote): session-agnostic flushing broadcasts duplicate PGO chunks; make flushing session-aware or only flush when appropriate. |
src/native/eventpipe/ep-rt.h |
Declares the lifecycle hook. nit (3 votes): correct the inaccurate EventPipe-lock contract comment. |
src/mono/wasm/Wasm.Build.Tests/Wasm.Build.Tests.csproj |
Includes dotnet-pgo in test payloads. |
src/mono/wasm/Wasm.Build.Tests/Blazor/EventPipeDiagnosticsTests.cs |
Adds end-to-end PGO validation. moderate (2 votes): use the trimmed linker directory. moderate (3 votes): assert BasicBlockIntCount data, not only method presence. |
src/mono/wasm/features.md |
Documents WASM PGO usage. nit (1 vote): align DLL identity guidance with the tool’s actual CodeView/PDB GUID validation. |
src/mono/nuget/Microsoft.NET.Sdk.WebAssembly.Pack/build/Microsoft.NET.Sdk.WebAssembly.Browser.CoreCLR.targets |
Integrates the browser CoreCLR build settings. |
src/mono/mono/eventpipe/ep-rt-mono.h |
Adds the Mono no-op lifecycle hook. |
src/coreclr/vm/pgo.h |
Declares PGO instrumentation flushing. |
src/coreclr/vm/pgo.cpp |
Flushes accumulated instrumentation data. |
src/coreclr/vm/jitinterface.h |
Exposes shared PGO interface methods. |
src/coreclr/vm/jitinterface.cpp |
Shares PGO allocation with the interpreter. moderate (1 vote): limit the tiering-gate relaxation to the interpreter callback. |
src/coreclr/vm/interpexec.cpp |
Executes PGO counter probes. moderate (1 vote): threaded builds can race on the counter; use synchronized counters or exclude them. moderate (2 votes): use the unsigned counter type to avoid signed overflow and match the schema. |
src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.h |
Adds the CoreCLR lifecycle hook declaration. |
src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.cpp |
Connects EventPipe stopping to PGO flushing. moderate (2 votes): prevent duplicate chunks when sessions overlap. |
src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h |
Adds the AOT no-op hook. |
src/coreclr/interpreter/interpconfigvalues.h |
Defines interpreter PGO settings. |
src/coreclr/interpreter/inc/intops.def |
Adds the PGO counter opcode. |
src/coreclr/interpreter/eeinterp.cpp |
Initializes interpreter PGO instrumentation. |
src/coreclr/interpreter/compiler.h |
Stores interpreter instrumentation state and helpers. |
src/coreclr/interpreter/compiler.cpp |
Emits block-head probes. moderate (1 vote): increment the unsigned BasicBlockIntCount counter with an unsigned type. |
src/coreclr/inc/clrconfigvalues.h |
Adds interpreter PGO configuration. |
src/coreclr/clrfeatures.cmake |
Enables PGO for WASM. |
Review details
Suppressed comments (7)
src/coreclr/interpreter/compiler.cpp:8757
BasicBlockIntCountis an unsigned four-byte counter (seecorjit.h/PgoFormat.cs), but this executes a signedint32_tincrement. A hot interpreted method can eventually overflowINT32_MAX, which is undefined behavior in C++, and the access does not match the schema's unsigned representation. Use auint32_t*(or an equivalent unsigned increment) here.
int32_t *pCounter = (int32_t*)(pInstrumentationData + pSchema[i].Offset);
src/coreclr/vm/interpexec.cpp:2071
INTOP_PGO_COUNTis compiled for threaded browser/WASI builds too:WasmEnableThreads=trueremovesPERFTRACING_DISABLE_THREADS, while this opcode is guarded only by the target. Multiple workers can race on this read-modify-write, and session stopping can read the same counter concurrently, so counts can be lost or undefined. Use an atomic/interlocked counter with a synchronized snapshot, or explicitly exclude threaded builds.
(*(int32_t*)pMethod->pDataItems[ip[1]])++;
src/coreclr/vm/jitinterface.cpp:13095
CEECodeGenInfois the common base of bothCEEJitInfoandCInterpreterJitInfo, so this condition also relaxes the JIT's tiering-eligibility gate wheneverDOTNET_InterpPGO=1. Any JIT PGO phase can then allocate instrumentation for non-tiering-eligible methods, and a later JIT schema can replace an interpreter schema for the same method inPgoManager. Keep the relaxation limited to the interpreter callback rather than this shared implementation.
// Only try instrumenting tiering-eligible methods, unless interpreter PGO is enabled, in
// which case we instrument every method for offline profile collection.
MethodDesc* pMD = (MethodDesc*)ftnHnd;
if (pMD->IsEligibleForTieredCompilation() || InterpreterPgoInstrumentationEnabled())
{
src/mono/wasm/features.md:471
- The conversion tool currently validates CodeView/PDB GUIDs (
src/coreclr/tools/dotnet-pgo/Program.cs:1304-1322) and explicitly notes that it does not match MVIDs (:1340). This documentation therefore attributesDll mismatchto an MVID check thatdotnet-pgodoes not perform; please align the guidance with the actual identity check (or update the tool and docs together) so users do not diagnose the wrong cause.
`--reference` must point at assemblies whose **MVID** matches the modules recorded in the trace, otherwise
`dotnet-pgo` reports `Dll mismatch ...` (or `Unknown ModuleID` for the affected methods). On browser/wasm
the assemblies loaded by the runtime are the **IL-trimmed** ones: `PublishTrimmed`/ILLink rewrites each
assembly and **generates a fresh MVID**, then those trimmed DLLs are converted to the fingerprinted
`*.wasm` files in `_framework` (webcil preserves the MVID byte-for-byte). So the trace records the
**trimmed** MVIDs, which do **not** match the untrimmed assemblies in the runtime pack
src/native/eventpipe/ep.c:808
ep_rt_session_stopping()is called for everystop_session(id), but the hook has no session ID andWritePgoData()uses the normal EventPipe write path. Those events are broadcast to every still-live session, so stopping an unrelated or earlier diagnostic session flushes the complete PGO dataset into this trace; the later PGO-session stop flushes it again.dotnet-pgorejects a new chunk after a method's final chunk and drops that method, making traces unreliable when sessions overlap. Make the hook session-aware/target the write, or flush only once when the final relevant session stops.
// Give the runtime a chance to emit any pending end-of-session data (e.g. block-count PGO)
// into the still-live session. This must run before taking the EventPipe lock: emitting events
// re-enters the write path, which requires the lock not be held.
ep_rt_session_stopping ();
src/native/libs/System.Native.Browser/diagnostics/diagnostic-server-js.ts:179
- The existing PGO test invokes
collectPgoTracefrom an already-running page, so it does not exercise this newjs://pgostartup registration. A failure increateDiagConnectionJsor thestartup=truesetup would leave the documented pre-managed-code capture broken while the test still passes. Add a startup-port case that verifies the downloaded trace.
if (scenarioName.startsWith("js://pgo")) {
collectPgoTrace({}, true);
src/native/libs/System.Native.Browser/diagnostics/dotnet-pgo-trace.ts:32
- The timeout callback is detached from the session it was created for and stops whatever session is currently in the global
pgoSession. If the first session closes early and a second trace starts before the first timeout fires, the stale timer will stop the second trace prematurely. Capture/check the original session before sending the stop command.
Module.safeSetTimeout(() => {
stopPgoTrace();
}, 1000 * durationSeconds);
- Files reviewed: 28/29 changed files
- Comments generated: 5
- Review effort level: Lite
collectPgoTrace now rejects a second collection after the first has run and flushed, since the interpreter block-count counters are cumulative and re-emitting them would produce a duplicate chunk sequence that dotnet-pgo drops. The latch is set only once a session actually started, so a setup that fails before starting still allows a retry.
# Conflicts: # src/mono/nuget/Microsoft.NET.Sdk.WebAssembly.Pack/build/Microsoft.NET.Sdk.WebAssembly.Browser.CoreCLR.targets
There was a problem hiding this comment.
🟡 Changes recommended
Critical build/test issues and unresolved WASI and block-count correctness concerns must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
src/coreclr/interpreter/compiler.cpp:8689
- This adds the block-count producer to
TARGET_WASI, but the current CoreCLR WASI configuration setsFEATURE_PERFTRACING=0insrc/coreclr/CMakeLists.txt:37-45; consequently the EventPipe hook andJitInstrumentationDataVerboseexport are not built, and WASI has no corresponding JS trigger. A WASI run can allocate and increment counters that can never become an.mibc; either keep this instrumentation browser-only until WASI diagnostics are enabled, or add the missing export path before claiming WASI support.
#if (defined(TARGET_BROWSER) || defined(TARGET_WASI)) && defined(PERFTRACING_DISABLE_THREADS)
// Instrument each basic block with a block-count PGO probe. The counters are allocated by
// allocPgoInstrumentationBySchema (native PgoManager memory), so they persist independently of
// EventPipe session lifetime; the accumulated profile is flushed to the trace as
// JitInstrumentationDataVerbose events, which dotnet-pgo consumes to build an .mibc.
src/coreclr/interpreter/compiler.cpp:8725
- This filter emits counts only for the entry and explicit branch targets, but the block-count consumer does not reconstruct omitted blocks:
fgGetProfileWeightForBasicBlockreturns zero when an IL offset has no schema entry (src/coreclr/jit/fgprofile.cpp:321-342), andfgIncorporateBlockCountsassigns that value to the block. Hot fall-through blocks and exception-handler entries will therefore be serialized as cold in the MIBC; emit every canonical real IL block or add reconstruction before producing the profile.
if (bb->ilOffset == 0 || isBranchTarget[bb->index])
blocks.Add(bb);
src/coreclr/interpreter/inc/intops.def:95
- The
TARGET_WASIbranch is not active in the current CoreCLR WASI build:src/coreclr/CMakeLists.txt:37-45setsFEATURE_PERFTRACING=0, andsrc/coreclr/interpreter/CMakeLists.txt:50-52definesPERFTRACING_DISABLE_THREADSonly when perf tracing is enabled. Consequently WASI emits noINTOP_PGO_COUNTand has no EventPipe flush path, so the advertised browser/WASI collection support is currently browser-only. Either enable the required WASI diagnostics plumbing or remove the WASI guard/claim until that follow-up lands.
#if (defined(TARGET_BROWSER) || defined(TARGET_WASI)) && defined(PERFTRACING_DISABLE_THREADS)
OPDEF(INTOP_PGO_COUNT, "pgo.count", 2, 0, 0, InterpOpLdPtr)
#endif
src/native/eventpipe/ep-rt.h:245
session_maskis not a keyword mask:ep_session_get_maskreturns the single session-routing bit (1 << session->index), which is exactly whatep_event_is_enabled_by_maskexpects. Calling it a keyword mask makes this hook's contract misleading and could cause a future implementation to pass provider keyword flags instead; describe it as the session bit/routing mask.
// is the session's keyword mask captured under the EventPipe lock, so the runtime can test provider
// keywords without dereferencing the session, which a concurrent stop may free once the lock is
- Files reviewed: 29/30 changed files
- Comments generated: 2
- Review effort level: Lite
|
If the idea is for JIT to be able to leverage this data, we need to pay careful attention to the schema formation. In the JIT, count reconstruction from sparse profiles currently only runs with edge profiling, and the schema used for this must be one the JIT can recreate from IL analysis (probably tricky to pull off). If you want to emit sparse block data we would need a new reconstruction algorithm in the JIT to try and infer the missing counts. Or maybe there is SPGO code in dotnet-pgo that can do likewise. That would free us from having to try and match the JIT's notion of basic block boundaries. If the JIT is not the intended consumer then we can ignore all that. Also note that class identity information can be quite useful (class histograms), as well as "value profiles". This is what lights up GDV and other advanced opts. |
Per EventPipe-owner feedback: bind the stopping session as the current thread's rundown session inside a new single-threaded-only session_stopping helper in ep.c (save/restore the previous binding under the config lock), and add a shared ep_event_is_enabled_for_current_thread helper. The CoreCLR session-stopping hook now just gates on that helper and emits, with session routing and validation owned by EventPipe; the hook reverts to a single session_id parameter. Rename PgoManager::EmitInstrumentationDataToEventPipe to LogInstrumentationData to match LogMethodInstrumentationData.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Five unresolved moderate findings remain across probe coverage, WASI support, R2R validation, test staging, and EventPipe handling.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
Resolved since last review (6)
MSBuild Targets="Build"does not populateTargetOutputsfor this project, so…WasmPerformanceInstrumentation=noneis the documented way to explicitly disable CPU…Builddoes not produce aTargetOutputsitem for this SDK project, so_DotnetPgoBuiltAssembly…commandCollectTracing2()(per the provided context) unconditionally calls…TargetOutputscommonly returns multiple items; assigning `@(_DotnetPgoBuiltAssembly->'%(RootDir)%(…TargetOutputscommonly returns multiple items; assigning `@(_DotnetPgoBuiltAssembly->'%(RootDir)%(…
Per review feedback, probe the same points as the sampling profiler - method entry and targets of backward branches - instead of every branch/switch/leave target, using a bit set on the target basic block in EmitBranch. Counters remain exact (bumped every execution, not sampled), giving exact method invocation and loop trip counts. Acyclic branch structure is deliberately left unprofiled: mapping interpreter blocks onto the JIT's is approximate, and block-count schemas get no flow reconstruction in the consumer, so that precision is deferred to the planned R2R-side instrumentation. Removes the per-instruction branch-target scan.
|
Thanks, this was the input that settled the design. My plan is that block-level precision comes from a follow-up that instruments R2R code itself, where the profile maps back onto the same IR that consumes it — no interpreter→JIT block mapping involved. For the interpreter (this PR) I went with the simplification @BrzVlad suggested: probe only method entry and loop heads (targets of backward branches — the same points the WASM sampling profiler uses), with exact counters rather than samples. That's a direct response to your point. The JIT is the consumer here (this feeds crossgen2 for R2R), and looking at the consumption path confirms your concern: Once R2R instrumentation lands, the interpreter-only residue is methods that can't be R2R-compiled at all — crossgen2 never compiles those, so their coarse counts are never used for codegen. And the cross-method signals ( On provenance: crossgen2 hardcodes Agreed on class histograms and value profiles for GDV — out of scope here, but worth doing once the collection pipeline is established. Note Reply drafted with GitHub Copilot. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved moderate issues affect WASI support, probe coverage, threaded collection behavior, configuration, and test packaging.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
The hook no longer takes a session id: the caller validates the session and binds the current thread to it, so the runtime identifies the target via ep_event_is_enabled_for_current_thread. Use the ep_thread_set_as_rundown_thread wrapper for both bind and restore, and unbind on the error path so a failed restore-lock acquisition cannot leave the thread scoped to a session.
| bool InterpCompiler::s_browserProfilerEnabled = false; | ||
| #endif | ||
| #endif // PERFTRACING_DISABLE_THREADS | ||
| #if (defined(TARGET_BROWSER) || defined(TARGET_WASI)) && defined(PERFTRACING_DISABLE_THREADS) |
There was a problem hiding this comment.
Remove this ifdef and leave it enabled on all platforms? It is only a small amount of code and it tends to be useful to be able to enable features like this for testing in environments that are easier to debug.
There was a problem hiding this comment.
Done in 3712dc2 — the ifdefs are gone and the instrumentation builds and runs on all platforms, opt-in behind DOTNET_InterpPGO.
Worth noting there was a second gate I had to remove for this to actually work: InterpreterPgoInstrumentationEnabled() in jitinterface.cpp was also #if TARGET_BROWSER || TARGET_WASI, so allocPgoInstrumentationBySchema returned E_NOTIMPL and no probes were emitted off-WASM. Both builds were green with that still in place — I only caught it by running it. Verified on Windows x64 with corerun + DOTNET_InterpPGO=1: probes are emitted at method entry and loop heads as expected, which is exactly the easier-to-debug environment you were after.
One question on the counter itself: I made the increment InterlockedIncrement, since without the single-threaded gate concurrent executions of an instrumented method would race. But the JIT's own probes default to racy (JitInterlockedProfiling defaults to 0, and ScalableApproximateCounting.md explains the overhead tradeoff). Happy to match that instead and document the counts as approximate if you'd rather keep the interpreter dispatch loop cheaper.
The EventPipe session-stopping flush stays single-threaded-only (the WASM-specific piece @lateralusX and I scoped to this PR), so on desktop the counters come out via the existing DOTNET_WritePGOData/DOTNET_PGODataPath text export at shutdown rather than over a trace.
🤖 Reply drafted with GitHub Copilot.
The isBackwardBranchTarget bit was only set in EmitBranch, so loop heads reached via CEE_SWITCH (which links targets directly) or EmitLeave (which calls EmitBranchToBB directly) received no PGO probe and their execution counts were absent from the profile. Mark both. For leave, mark before the finally-call-island redirection so the bit lands on the real IL block rather than an island that shares its IL offset.
Per review feedback, drop the browser/WASI single-threaded ifdefs around the interpreter block-count instrumentation so the feature can be exercised on desktop, where it is much easier to debug. This includes the VM-side InterpreterPgoInstrumentationEnabled gate, without which allocPgoInstrumentationBySchema returns E_NOTIMPL and no probes are emitted off-WASM. The feature stays opt-in behind DOTNET_InterpPGO, and the counter increment is now interlocked so concurrent executions of an instrumented method do not lose counts. The EventPipe session-stopping flush remains single-threaded-only; on other platforms the counters are collected through the existing DOTNET_WritePGOData text export at shutdown.



Summary
Instruments the CoreCLR interpreter with block-count PGO probes on WebAssembly and adds a JavaScript trigger to collect the profile over EventPipe, so
dotnet-pgocan produce an.mibcfor R2R precompilation. This is the profile production side of PGO-on-WebAssembly; consumption (crossgen2 on WASM) is tracked separately.This targets the single-threaded browser/WASI interpreter (the offline PGO-collection config,
PERFTRACING_DISABLE_THREADS); the feature is compiled out on multithreaded WASM.Part of #130524. Implements #130517 and #130518.
Instrumentation (#130517)
INTOP_PGO_COUNTinterpreter opcode (single-threaded browser/WASI only) that increments a nativeuint32_tcounter allocated viaallocPgoInstrumentationBySchema, so counters outlive the EventPipe session and wrap as the profile format expects.InterpCompiler::InstrumentBlockCountsemitsBasicBlockIntCountprobes at block heads only — method entry plus branch/switch/loop targets, restricted to the original IL range (m_ILCodeSizeFromILHeader, so synthetic finally/epilog IL for synchronized/async methods is skipped) — gated byDOTNET_InterpPgowith an optionalDOTNET_InterpPgoMethodsmethod filter.alloc*/get*PGO interface methods move to the sharedCEECodeGenInfobase so the JIT and interpreter share one implementation; the tiering gate is relaxed for the interpreter, target-scoped to browser/WASI.FEATURE_PGOis enabled for WASM independently.PERFTRACING_DISABLE_THREADS, so multithreaded (WasmEnableThreads) builds never emit the counter and can't race on the increment.Flush over EventPipe
JitInstrumentationDataVerboseevents on EventPipe session stop via a newep_rt_session_stoppinghook. CoreCLR callsPgoManager::EmitInstrumentationDataToEventPipe()(Mono and NativeAOT are no-ops). The hook runs before the EventPipe lock is taken, since emitting events re-enters the write path; the stopping session's keyword mask is captured under the lock instop_sessionand passed to the hook (ep_rt_session_stopping(id, session_mask)), so the runtime tests the keyword without dereferencing a session a concurrent stop could free.EmitInstrumentationDataToEventPipe()only fires the events; theDOTNET_WritePGODatatext dump stays inWritePgoData(), driven solely by the process-shutdown path — an on-demand trace collection never writes the text file.WritePgoData()emits to EventPipe only under!PERFTRACING_DISABLE_THREADS. On single-threaded WASM the session-stopping hook is the sole EventPipe emitter, so a method is never delivered twice into a session EventPipe stops during shutdown (whichdotnet-pgorejects as a duplicate chunk after a method's final chunk); threaded desktop still emits at shutdown as before.ep_session_write_eventinstead of broadcasting to every enabled session (the same mechanism EventPipe uses for method/assembly rundown at teardown).PORTABILITY_ASSERT, gated on the stopping session'sJitInstrumentationDatakeyword, flags a genuine PGO-collection attempt on that unsupported config without tripping on unrelated (CPU/GC/counters) sessions.JS trigger (#130518)
collectPgoTrace()diagnostic client (js://pgo) starts a trace with theJitInstrumentationDatakeyword — mask aligned to the IBC keyword setdotnet-pgoconsumes — and auto-downloads the.nettraceafter a default 10s window. The stop timer only stops the session it started.collectPgoTraceis rejected rather than re-emitting cumulative data thatdotnet-pgowould drop as a restarted chunk sequence. Restart the app to collect again.Notes
dotnet-pgomust reference the IL-trimmedlinked/*.dll(whose MVID matches the running app), not the untrimmed runtime pack.src/mono/wasm/features.md.Validation
.nettracewithJitInstrumentationDataVerboseevents →dotnet-pgo→ valid.mibc.Note
This PR description was drafted with GitHub Copilot.